Feat/get config UI tracking#367
Conversation
Introduce a dedicated config editor resource with structured get_config payloads and shared host-context/compact-row infrastructure so MCP UIs behave consistently across hosts while tightening config key validation and preview type coverage.
Validate blank numeric input, keep array modal open on save failures, bind tool-bridge listeners safely, and tune host-driven toggle styling/expansion behavior for embedded hosts.
Use config-manager telemetry flag helpers for GA/proxy gating and add a small regression test that verifies string 'false' handling across set_value and set_config_value.
Add a shared UI event tracker utility, wire config editor and file preview to it, and capture set_config_value call origin for telemetry analytics.
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughThis PR adds telemetry value normalization and disabled-state checks, integrates a reusable UI event tracker into the config editor and file preview, enforces boolean coercion/validation for config writes, records config call origin in telemetry, and adds tests covering telemetry behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Config Editor
participant Controller as Config Controller
participant Tracker as UiEventTracker
participant Tool as ToolCaller
participant Server as Server/Config Manager
participant Telemetry as Telemetry Proxy
UI->>Controller: user updates config
Controller->>Tracker: trackConfigUiEvent('config_update', params)
Tracker->>Tool: callTool('track_ui_event', {...baseParams, ...params})
Tool->>Server: call set_config_value(args + origin)
Server->>Server: normalize/store config (normalizeTelemetryEnabledValue)
Server->>Telemetry: capture event (gated via isTelemetryDisabledValue)
alt success
Server-->>Controller: update success
Controller->>Tracker: trackConfigUiEvent('config_update_success')
else failure
Server-->>Controller: error
Controller->>Tracker: trackConfigUiEvent('config_update_failed', {error: sanitized})
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:L
Sequence DiagramShows the core flow where a user updates a config via the UI; the Config Editor calls set_config_value (marked as originating from the UI), the server applies the change via ConfigManager which normalizes boolean telemetry inputs and emits a final telemetry opt-out event that is gated by telemetry helpers. sequenceDiagram
participant User
participant ConfigEditor
participant Server
participant ConfigManager
participant Capture
User->>ConfigEditor: Save config (telemetryEnabled = "false")
ConfigEditor->>Server: callTool set_config_value { key, value, origin: "ui" }
Server->>ConfigManager: setValue(key="telemetryEnabled", value="false")
ConfigManager->>ConfigManager: normalizeTelemetryEnabledValue -> false
alt telemetry transitioning to disabled
ConfigManager->>Capture: capture('server_telemetry_opt_out', { prev_value, reason })
Capture->>Capture: isTelemetryDisabledValue checks config; gates sending
end
ConfigManager-->>Server: setValue result
Server-->>ConfigEditor: success (updated config)
Generated by CodeAnt AI |
Nitpicks 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/ui/shared/ui-event-tracker.ts (1)
35-38: Protect base telemetry context from accidental overrides.Per-event params currently override
baseParams. For stable context fields, prefer base params taking precedence.♻️ Proposed adjustment
params: { - ...baseParams, ...normalizeUiEventParams(params), + ...baseParams, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/shared/ui-event-tracker.ts` around lines 35 - 38, The merge currently spreads baseParams then normalizeUiEventParams(params) so per-event fields can override the shared context; update the merge in the params construction inside ui-event-tracker.ts to ensure baseParams have precedence (i.e., merge per-event params first then baseParams or use Object.assign with per-event then base), referencing the existing symbols baseParams and normalizeUiEventParams(params) and the params object so stable context fields from baseParams cannot be accidentally overridden by per-event values.src/ui/config-editor/src/app.ts (1)
465-483: Consider emittingconfig_update_successonly after refresh succeeds.Current ordering can report success even when the follow-up refresh fails and UI shows an error.
♻️ Proposed adjustment
- trackConfigUiEvent?.('config_update_success', { - tool_name: 'set_config_value', - ...buildConfigUpdateTelemetryParams({ - configKey: selected.key, - valueType: selected.valueType, - }), - }); - const refreshed = await callTool('get_config', {}); if (isToolErrorResult(refreshed)) { const errorMessage = extractToolText(refreshed) ?? 'Value was updated but config refresh failed.'; return { ok: false, tooltip: { message: errorMessage, tone: 'error', }, }; } @@ + trackConfigUiEvent?.('config_update_success', { + tool_name: 'set_config_value', + ...buildConfigUpdateTelemetryParams({ + configKey: selected.key, + valueType: selected.valueType, + }), + }); + return { ok: true, };Also applies to: 496-499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/config-editor/src/app.ts` around lines 465 - 483, Move the telemetry success emission so it only runs after the config refresh succeeds: call callTool('get_config', {}) and check with isToolErrorResult(refreshed) and extractToolText(refreshed) first, and only if the refresh is not an error invoke trackConfigUiEvent('config_update_success', { tool_name: 'set_config_value', ...buildConfigUpdateTelemetryParams({ configKey: selected.key, valueType: selected.valueType }) }). Apply the same change to the other occurrence that currently emits success before refresh (the block using trackConfigUiEvent, callTool, isToolErrorResult, and extractToolText around the second update path).test/test-telemetry-handling.js (1)
26-53: Consider expanding coercion tests beyond the'false'path.Current tests validate one branch only. Add
'true'and invalid-string cases to lock behavior for both success and error paths.Suggested additions
async function testConfigManagerCoercion() { console.log('\n--- Test: configManager telemetry coercion ---'); await configManager.updateConfig({ telemetryEnabled: false }); await configManager.setValue('telemetryEnabled', 'false'); const telemetryEnabled = await configManager.getValue('telemetryEnabled'); assert.strictEqual(telemetryEnabled, false); assert.strictEqual(typeof telemetryEnabled, 'boolean'); + + await configManager.setValue('telemetryEnabled', 'true'); + const telemetryEnabledTrue = await configManager.getValue('telemetryEnabled'); + assert.strictEqual(telemetryEnabledTrue, true); + assert.strictEqual(typeof telemetryEnabledTrue, 'boolean'); console.log('ok: configManager coercion'); }async function testSetConfigValueCoercion() { console.log('\n--- Test: set_config_value telemetry coercion ---'); await configManager.updateConfig({ telemetryEnabled: false }); const response = await setConfigValue({ key: 'telemetryEnabled', value: 'false' }); assert.ok(response); assert.notStrictEqual(response.isError, true); const telemetryEnabled = await configManager.getValue('telemetryEnabled'); assert.strictEqual(telemetryEnabled, false); assert.strictEqual(typeof telemetryEnabled, 'boolean'); + + const responseTrue = await setConfigValue({ key: 'telemetryEnabled', value: 'true' }); + assert.notStrictEqual(responseTrue.isError, true); + assert.strictEqual(await configManager.getValue('telemetryEnabled'), true); + + const responseInvalid = await setConfigValue({ key: 'telemetryEnabled', value: 'disabled' }); + assert.strictEqual(responseInvalid.isError, true); console.log('ok: set_config_value coercion'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-telemetry-handling.js` around lines 26 - 53, Add test cases for both the true path and invalid-string error path to the existing coercion tests: within testConfigManagerCoercion and testSetConfigValueCoercion (and/or new tests named e.g., testConfigManagerCoercion_true and testConfigManagerCoercion_invalid), set telemetryEnabled to 'true' and assert the resulting value is boolean true, then set telemetryEnabled to an invalid string (like 'notabool') and assert the code either returns an error or preserves the prior value depending on intended behavior; for setConfigValue use the same three inputs ('false', 'true', and an invalid string) and assert response.isError and configManager.getValue results accordingly so both success and error branches are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test-telemetry-handling.js`:
- Around line 56-57: The test currently assigns originalConfig = await
configManager.getConfig() which is only a shallow copy and allows nested
arrays/objects (e.g., blockedCommands) to be mutated by tests; replace the
shallow snapshot with a deep defensive clone by calling structuredClone on the
value returned from configManager.getConfig() (and likewise use structuredClone
when preparing the restore at setConfig/restore code around originalConfig) so
the test's originalConfig is immutable to nested mutations.
- Around line 77-86: The direct comparison of import.meta.url to
`file://${process.argv[1]}` can fail due to URL encoding and platform path
differences; update the entrypoint check to convert import.meta.url with
fileURLToPath and normalize both sides with path.resolve before comparing.
Specifically, import and use fileURLToPath from 'url' and path.resolve from
'path', then compare path.resolve(fileURLToPath(import.meta.url)) ===
path.resolve(process.argv[1]) (the block surrounding runTests() should remain
the same) so the check is robust across platforms and encoded characters.
---
Nitpick comments:
In `@src/ui/config-editor/src/app.ts`:
- Around line 465-483: Move the telemetry success emission so it only runs after
the config refresh succeeds: call callTool('get_config', {}) and check with
isToolErrorResult(refreshed) and extractToolText(refreshed) first, and only if
the refresh is not an error invoke trackConfigUiEvent('config_update_success', {
tool_name: 'set_config_value', ...buildConfigUpdateTelemetryParams({ configKey:
selected.key, valueType: selected.valueType }) }). Apply the same change to the
other occurrence that currently emits success before refresh (the block using
trackConfigUiEvent, callTool, isToolErrorResult, and extractToolText around the
second update path).
In `@src/ui/shared/ui-event-tracker.ts`:
- Around line 35-38: The merge currently spreads baseParams then
normalizeUiEventParams(params) so per-event fields can override the shared
context; update the merge in the params construction inside ui-event-tracker.ts
to ensure baseParams have precedence (i.e., merge per-event params first then
baseParams or use Object.assign with per-event then base), referencing the
existing symbols baseParams and normalizeUiEventParams(params) and the params
object so stable context fields from baseParams cannot be accidentally
overridden by per-event values.
In `@test/test-telemetry-handling.js`:
- Around line 26-53: Add test cases for both the true path and invalid-string
error path to the existing coercion tests: within testConfigManagerCoercion and
testSetConfigValueCoercion (and/or new tests named e.g.,
testConfigManagerCoercion_true and testConfigManagerCoercion_invalid), set
telemetryEnabled to 'true' and assert the resulting value is boolean true, then
set telemetryEnabled to an invalid string (like 'notabool') and assert the code
either returns an error or preserves the prior value depending on intended
behavior; for setConfigValue use the same three inputs ('false', 'true', and an
invalid string) and assert response.isError and configManager.getValue results
accordingly so both success and error branches are covered.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/config-manager.tssrc/server.tssrc/tools/config.tssrc/tools/schemas.tssrc/ui/config-editor/src/app.tssrc/ui/file-preview/src/app.tssrc/ui/shared/ui-event-tracker.tssrc/utils/capture.tstest/test-telemetry-handling.js
| const originalConfig = await configManager.getConfig(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'config-manager' --type fRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 97
🏁 Script executed:
cat -n src/config-manager.ts | head -150Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 5884
🏁 Script executed:
rg -n "getConfig|updateConfig|saveConfig" src/config-manager.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 376
🏁 Script executed:
sed -n '191,200p' src/config-manager.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 300
🏁 Script executed:
cat -n test/test-telemetry-handling.js | sed -n '50,85p'Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1308
🏁 Script executed:
sed -n '242,260p' src/config-manager.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 574
🏁 Script executed:
cat -n test/test-telemetry-handling.js | sed -n '1,55p'Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 2432
Use structuredClone() to defensively snapshot config and prevent reference-mutation leakage.
getConfig() returns a shallow copy ({ ...this.config }), so nested objects and arrays like blockedCommands remain as references to the original. If any test mutates these nested structures, the snapshot in originalConfig would also be mutated, making the restore at line 73 ineffective. Use structuredClone() for a deep copy that protects against this.
Suggested hardening
export default async function runTests() {
- const originalConfig = await configManager.getConfig();
+ const originalConfig = structuredClone(await configManager.getConfig());Also applies to: 73-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/test-telemetry-handling.js` around lines 56 - 57, The test currently
assigns originalConfig = await configManager.getConfig() which is only a shallow
copy and allows nested arrays/objects (e.g., blockedCommands) to be mutated by
tests; replace the shallow snapshot with a deep defensive clone by calling
structuredClone on the value returned from configManager.getConfig() (and
likewise use structuredClone when preparing the restore at setConfig/restore
code around originalConfig) so the test's originalConfig is immutable to nested
mutations.
| if (import.meta.url === `file://${process.argv[1]}`) { | ||
| runTests() | ||
| .then((success) => { | ||
| process.exit(success ? 0 : 1); | ||
| }) | ||
| .catch((error) => { | ||
| console.error('Unhandled error:', error); | ||
| process.exit(1); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/test-telemetry-handling.js | head -100Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 3479
🌐 Web query:
Node.js ESM import.meta.url comparison process.argv[1] platform issues
💡 Result:
Why import.meta.url vs process.argv[1] can be flaky across platforms
- They’re different “kinds” of identifiers:
import.meta.urlis afile:URL string, whileprocess.argv[1]is an absolute filesystem path string (Node resolves it viapath.resolve). (2ality.com) - URL encoding vs raw paths: ESM modules are resolved/cached as URLs, so characters may be percent-encoded in
import.meta.url(e.g.#→%23), whileprocess.argv[1]is not. (nodejs.org) - Windows-specific traps:
- If you do
new URL(import.meta.url).pathname, you’ll often get a leading slash like/C:/..., which is not a valid Windows path; usefileURLToPath()instead. (bero.dev) file:URLs with hostnames can map to UNC paths (\\server\share\...). (r2.nodejs.org)
- If you do
- “Main module” detection edge cases: comparing strings can fail if the script is invoked in different ways (e.g., extensionless invocation, wrappers, etc.). (stackoverflow.com)
Recommended solutions
1) Best (Node.js ≥ v24.2.0): import.meta.main
Node now provides a built-in boolean for “is this the entry point?” (nodejs.org)
if (import.meta.main) {
// run as: node yourfile.mjs
}2) Portable workaround (older Node): normalize to the same representation
Convert the CLI path to a file URL (or convert import.meta.url to a path) before comparing:
import { pathToFileURL, fileURLToPath } from 'node:url';
import { resolve } from 'node:path';
const isMain =
process.argv[1] &&
import.meta.url === pathToFileURL(resolve(process.argv[1])).href;
// or:
// resolve(process.argv[1]) === fileURLToPath(import.meta.url)This avoids Windows pathname pitfalls and compares like-with-like. (2ality.com)
Citations:
- 1: https://2ality.com/2022/07/nodejs-esm-main.html?utm_source=openai
- 2: https://nodejs.org/download/release/v23.11.1/docs/api/esm.html?utm_source=openai
- 3: https://bero.dev/dont-use-url-to-construct-paths-in-node-js/?utm_source=openai
- 4: https://r2.nodejs.org/docs/v12.8.1/api/fs.html?utm_source=openai
- 5: https://stackoverflow.com/questions/45136831/node-js-require-main-module?utm_source=openai
- 6: https://nodejs.org/download/release/latest-v24.x/docs/api/esm.html?utm_source=openai
- 7: https://2ality.com/2022/07/nodejs-esm-main.html?utm_source=openai
Use path-normalized direct-execution check for ESM entrypoints.
Line 77 directly compares import.meta.url (a file: URL with potential percent-encoding) against a constructed URL string, which can fail across platforms and with URL-encoded characters (e.g., # → %23). Normalize both to resolved filesystem paths using fileURLToPath() + path.resolve().
Suggested fix
import assert from 'assert';
+import path from 'node:path';
+import { fileURLToPath } from 'node:url';
@@
-if (import.meta.url === `file://${process.argv[1]}`) {
+const isDirectRun =
+ typeof process.argv[1] === 'string' &&
+ path.resolve(fileURLToPath(import.meta.url)) === path.resolve(process.argv[1]);
+
+if (isDirectRun) {
runTests()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (import.meta.url === `file://${process.argv[1]}`) { | |
| runTests() | |
| .then((success) => { | |
| process.exit(success ? 0 : 1); | |
| }) | |
| .catch((error) => { | |
| console.error('Unhandled error:', error); | |
| process.exit(1); | |
| }); | |
| } | |
| const isDirectRun = | |
| typeof process.argv[1] === 'string' && | |
| path.resolve(fileURLToPath(import.meta.url)) === path.resolve(process.argv[1]); | |
| if (isDirectRun) { | |
| runTests() | |
| .then((success) => { | |
| process.exit(success ? 0 : 1); | |
| }) | |
| .catch((error) => { | |
| console.error('Unhandled error:', error); | |
| process.exit(1); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/test-telemetry-handling.js` around lines 77 - 86, The direct comparison
of import.meta.url to `file://${process.argv[1]}` can fail due to URL encoding
and platform path differences; update the entrypoint check to convert
import.meta.url with fileURLToPath and normalize both sides with path.resolve
before comparing. Specifically, import and use fileURLToPath from 'url' and
path.resolve from 'path', then compare
path.resolve(fileURLToPath(import.meta.url)) === path.resolve(process.argv[1])
(the block surrounding runTests() should remain the same) so the check is robust
across platforms and encoded characters.
|
CodeAnt AI finished reviewing your PR. |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
Sequence DiagramShows the main success path when the UI updates a config (including origin marking) and how telemetry opt-out is normalized and emits a final opt-out event that is gated by the telemetry-disabled helper added in this PR. sequenceDiagram
participant UI
participant Server
participant ConfigManager
participant CaptureService
UI->>Server: callTool set_config_value { key, value, origin: "ui" }
Server->>ConfigManager: setValue(key, value) (preserves origin metadata)
ConfigManager->>ConfigManager: normalizeTelemetryEnabledValue(value) (coerce "true"/"false")
alt value becomes disabled (false)
ConfigManager->>CaptureService: capture('server_telemetry_opt_out', { prev_value })
CaptureService-->>ConfigManager: (checks isTelemetryDisabledValue and sends if allowed)
end
ConfigManager-->>Server: OK (value saved)
Server-->>UI: success response
Generated by CodeAnt AI |
|
CodeAnt AI Incremental review completed. |
2 participants
User description
Summary by CodeRabbit
New Features
Bug Fixes
Tests
CodeAnt-AI Description
Add reliable telemetry opt-out, UI event tracking, and robust boolean config inputs
What Changed
Impact
✅ Clearer config validation errors for boolean fields✅ Fewer accidental telemetry sends after user opt-out✅ Clearer analytics attribution for UI-driven config changes💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.